Skip to content

Replace panel ::before element with scroll-margin-top#1603

Merged
ang-zeyu merged 1 commit into
MarkBind:masterfrom
jonahtanjz:Fix-blocking-panel-element
Jun 27, 2021
Merged

Replace panel ::before element with scroll-margin-top#1603
ang-zeyu merged 1 commit into
MarkBind:masterfrom
jonahtanjz:Fix-blocking-panel-element

Conversation

@jonahtanjz

@jonahtanjz jonahtanjz commented Jun 27, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

What is the purpose of this pull request?

  • Documentation update
  • Bug fix
  • Feature addition or enhancement
  • Code maintenance
  • Others, please explain:

Overview of changes:
Resolves #1601.

By using ::before to create spacing before a panel, this blocks certain elements before the panel which will hinder interactive elements (buttons not clickable etc.). The spacing is needed so that panels do not get blocked by the fixed-header when using anchor navigation.

Replaced ::before with scroll-margin-top which similarly creates spacing before the panels when using anchor navigation and also do not block other elements.

Anything you'd like to highlight / discuss:
If there is a better workaround.

Testing instructions:

  1. Create panel with header
<panel header="# Test">
</panel>
  1. Click on panel link in page-nav
  2. Panel should appear below the fixed-header
  3. Can refer to User Guide: Unable to open details panel on mobile #1601 issue, visit nav bar section in UG, details panel should be clickable now

Proposed commit message: (wrap lines at 72 characters)
Fix blocking ::before pseudo element in panel

Using ::before in panels may block interactive elements before
the panels. The spacing created using ::before is needed so that
panels do not get blocked by the fixed header when using anchor
navigation.

Let's replace ::before with scroll-margin-top which similarly creates
spacing before the panels when using anchor navigation and also do
not block other elements.

Checklist: ☑️

  • Updated the documentation for feature additions and enhancements
  • Added tests for bug fixes or features
  • Linked all related issues
  • No blatantly unrelated changes
  • Pinged someone for a review!

@jonahtanjz jonahtanjz changed the title Replace panel before element with scroll-margin-top Replace panel ::before element with scroll-margin-top Jun 27, 2021
@ang-zeyu ang-zeyu merged commit a00655c into MarkBind:master Jun 27, 2021
@ang-zeyu ang-zeyu added this to the 3.0.3 milestone Jun 27, 2021
@ang-zeyu

ang-zeyu commented Jun 27, 2021

Copy link
Copy Markdown
Contributor

Thanks @jonahtanjz! Didn't know this property existed (was going to suggest width: 1px or avoid assigning the ids to the <div> version of panels).

We can probably explore this as an alternative to the dummy <span>s inserted for headings with ids as well (or even universally applying this to everything under a fixed-header-padding container with an id).

@ang-zeyu

ang-zeyu commented Jun 27, 2021

Copy link
Copy Markdown
Contributor

edit: might have jumped the gun here a little after getting excited with this property 😅 --

wonder if we should revert this for now and keep in view, seems like support for it on recent ios releases isn't too good yet (
https://caniuse.com/?search=scroll-margin-top) Scroll to fragment which is what we're depending on here especially is also bugged apparently, even with its in house scroll-snap-margin-top property.

@jonahtanjz

jonahtanjz commented Jun 27, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

wonder if we should revert this for now and keep in view, seems like support for it on recent ios releases isn't too good yet (
https://caniuse.com/?search=scroll-margin-top) Scroll to fragment which is what we're depending on here especially is also bugged apparently, even with its in house scroll-snap-margin-top property.

Hmm alright didn't know it was bugged on ios. Will raise another PR to temporary fix this using one of the suggestions. Yup we can probably wait till it is fully supported then replace the existing approach to fixed-header-padding.

@jonahtanjz jonahtanjz mentioned this pull request Jun 28, 2021
Merged
10 tasks
@jonahtanjz jonahtanjz mentioned this pull request Feb 14, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ang-zeyu ang-zeyu ang-zeyu approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

v3.0.3

Development

Successfully merging this pull request may close these issues.

User Guide: Unable to open details panel on mobile

2 participants

@jonahtanjz @ang-zeyu